Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for 'url' to --porcelain to output the URL instead of attachment ID #182

Merged

Conversation

justinmaurerdotdev
Copy link
Contributor

@justinmaurerdotdev justinmaurerdotdev commented Aug 24, 2023

This addresses #94
Should output ONLY the URL, instead of attachment ID or verbose descriptions.

Is that the best name for the flag?

Should we also provide an option for showing the URL in the verbose output?

Related wp-cli/wp-cli#5832

@justinmaurerdotdev justinmaurerdotdev requested a review from a team as a code owner August 24, 2023 19:36
src/Media_Command.php Outdated Show resolved Hide resolved
@danielbachhuber
Copy link
Member

Is that the best name for the flag?

Let's use --show-url, similar to --show-password for wp user reset-password

…ge URLs being returned by `wp_get_attachment_url()`. Improved tests, including non-image file for thoroughness.
@justinmaurerdotdev
Copy link
Contributor Author

One more question before I commit the last fixes: What I've built here is an alternative to the --porcelain output (which outputs attachment ID only) to output only the URL. Should we also provide a way to include the URL in the verbose/default output?

One idea would be to allow the --show-url flag to be added to the default setup OR combined with --porcelain, so that it would add a verbose output in the standard setup, or override the attachment ID in the --porcelain output.

With --show-url and without --porcelain, "Imported file '%s' as attachment ID %d%s." could become "Imported file '%s' as attachment ID %d%s to URL %s."

With --show-url AND --porcelain, only the URL would be shown.

Does that make sense? Or should we limit this to be simply an alternative to the --porcelain output?

@danielbachhuber
Copy link
Member

@justinmaurerdotdev I don't think we need to change the default output. However, I have a better idea for a flag:

[--porcelain[=<field>]]

This is how it would work:

$ wp media import large-image.jpg --porcelain`
45
$ wp media import large-image.jpg --porcelain=url
https://example.com/wp-content/uploads/large-image.jpg

We don't need to support all fields right now, but this approach gives us the flexibility to apply the pattern across other commands.

@justinmaurerdotdev
Copy link
Contributor Author

OK, yeah. That's perfect! Will push those changes shortly.

…the porcelain output to use additional fields. 'ID' (default) and 'url' currently supported.
src/Media_Command.php Outdated Show resolved Hide resolved
@danielbachhuber danielbachhuber changed the title Added '--porcelain_url' flag to output URL instead of attachment ID Add support for 'url' to --porcelain to output the URL instead of attachment ID Aug 28, 2023
Copy link
Member

@danielbachhuber danielbachhuber left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work on this! I made some minor stylistic changes with 0ca8360

@danielbachhuber danielbachhuber merged commit aa6d3f6 into wp-cli:main Aug 28, 2023
30 checks passed
@justinmaurerdotdev
Copy link
Contributor Author

Good deal. Changes look good. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants